fix: Force HTTPS actually stops the plaintext response - #876
Merged
blaipr merged 2 commits intoAug 26, 2026
Conversation
`Http::checkHttps()` sent a bare `header('Location: …')` — no status code, no exit, no
check that headers had already gone — and then returned. Both entry points called it and
carried straight on: `Web\Init` through the install and database checks to controller
dispatch, `Api\Init` likewise. The response was built and sent over the plaintext
connection the setting exists to refuse, and since a `Location` on a 200 is not a
redirect, no browser acted on the header either. Turning "Force HTTPS" on changed nothing
except adding an inert header.
The sibling refusals in the same method have always done it properly — not installed,
database unreachable, maintenance mode each redirect through the router and *then throw*.
That throw is the part that was missing, and it is why this now lives beside them:
`Http::httpsUrlFor()` answers with the address, and
`HttpModuleBase::redirectToHttpsIfRequired()` sends it and stops the request, in the base
both entry points already extend.
Also: the host was rewritten with `str_replace('http', 'https', …)`, which replaces every
occurrence — an installation at http://httpbin.example was redirected to
https://httpsbin.example, a host that need not exist and need not be theirs. Only the
scheme is rewritten now.
The three existing tests asserted which mock methods had been called and nothing else,
which is exactly why this survived: a method that calls isHttpsEnabled(), isHttps(),
getServerPort() and getHttpHost() satisfies all three whether or not it does anything
useful with them. They assert the address now, and three more assert what the base does
with it — that a plaintext request is redirected *and* stopped, and that an HTTPS request
and an installation with the setting off are both left alone, so a base that refused
everything would not pass.
Writing that test found a fatal in the first version of this change: `logger()` is a
global function in namespace `SP`, and `HttpModuleBase` is in `SP\Infrastructure`, so the
bare call resolved to nothing and every redirect would have died on it. It is imported.
…t in its tests too The integration suite caught the consequence of making the redirect actually stop the request: ConfigSecurityTest starts from an installation that already has "Force HTTPS" on and then dispatches over plain HTTP, which is exactly the case Init now refuses — so the controller never ran and nothing was saved. The test's premise is what changed, not its assertion. An installation requiring HTTPS is reached over HTTPS, so the request says so now. `buildRequest()` grows an optional server array for it, merged last so a test can state something the defaults do not. Only this one test needed it; nothing else in the suite turns the setting on.
blaipr
deleted the
fix/force-https-actually-stops-the-plaintext-response
branch
August 26, 2026 11:37
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Http::checkHttps()sent a bareheader('Location: …')— no status code, no exit, nocheck that headers had already gone — and then returned. Both entry points called it and
carried straight on:
Web\Initthrough the install and database checks to controllerdispatch,
Api\Initlikewise. The response was built and sent over the plaintextconnection the setting exists to refuse, and since a
Locationon a 200 is not aredirect, no browser acted on the header either. Turning "Force HTTPS" on changed nothing
except adding an inert header.
The sibling refusals in the same method have always done it properly — not installed,
database unreachable, maintenance mode each redirect through the router and then throw.
That throw is the part that was missing, and it is why this now lives beside them:
Http::httpsUrlFor()answers with the address, andHttpModuleBase::redirectToHttpsIfRequired()sends it and stops the request, in the baseboth entry points already extend.
Also: the host was rewritten with
str_replace('http', 'https', …), which replaces everyoccurrence — an installation at http://httpbin.example was redirected to
https://httpsbin.example, a host that need not exist and need not be theirs. Only the
scheme is rewritten now.
The three existing tests asserted which mock methods had been called and nothing else,
which is exactly why this survived: a method that calls isHttpsEnabled(), isHttps(),
getServerPort() and getHttpHost() satisfies all three whether or not it does anything
useful with them. They assert the address now, and three more assert what the base does
with it — that a plaintext request is redirected and stopped, and that an HTTPS request
and an installation with the setting off are both left alone, so a base that refused
everything would not pass.
Writing that test found a fatal in the first version of this change:
logger()is aglobal function in namespace
SP, andHttpModuleBaseis inSP\Infrastructure, so thebare call resolved to nothing and every redirect would have died on it. It is imported.